-
Notifications
You must be signed in to change notification settings - Fork 6
USB #63
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
USB #63
Conversation
examples/usb_serial.rs
Outdated
} | ||
|
||
// Echo back in upper case | ||
for c in buf[0..count].iter_mut() { | ||
if 0x61 <= *c && *c <= 0x7a { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any ideas for solving this?
error: current MSRV (Minimum Supported Rust Version) is `1.78.0` but this item is stable since `1.87.0`
--> examples/usb_serial.rs:72:32
|
72 | if let Ok(s) = str::from_utf8(&buf[0..count]) {
| ^^^^^^^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#incompatible_msrv
note: the lint level is defined here
--> examples/usb_serial.rs:2:9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could bump up the MSRV? We haven't published a crate yet...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, instead of doing this uppercasing manually, you could just call c.to_ascii_uppercase()
: https://doc.rust-lang.org/stable/core/primitive.char.html#method.to_ascii_uppercase (and use map
instead of a for loop)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh nice: )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any suggestions for a MSRV?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might as well bump it to 1.88?
Note that this is only tested on a Nucleo-H533RE |
src/usb.rs
Outdated
// Enable USB peripheral | ||
rcc.apb2enr().modify(|_, w| w.usben().set_bit()); | ||
|
||
// Reset USB peripheral | ||
rcc.apb2rstr().modify(|_, w| w.usbrst().set_bit()); | ||
rcc.apb2rstr().modify(|_, w| w.usbrst().clear_bit()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about:
rec::Usb { _marker: PhantomData }.reset().enable();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks
src/usb.rs
Outdated
|
||
fn enable() { | ||
cortex_m::interrupt::free(|_| { | ||
let rcc = unsafe { &*stm32::RCC::ptr() }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: move this closer to where it's first used.
Note: I have not tested this since b6c7432 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more comments
src/usb.rs
Outdated
/// Data negative pin | ||
pin_dm: DmPin, | ||
/// Data positive pin | ||
pin_dp: DpPin, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to hang on to these pins? Doesn't looks like they get used anywhere. Could just consume them in the new function so they can't get used anywhere else (e.g. SPI driver does this).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to hang on to these pins? [...]
I dont think we do
src/usb.rs
Outdated
impl fmt::Debug for UsbDevice { | ||
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
f.debug_struct("Peripheral") | ||
.field("usb", &"USB") | ||
.field("pin_dm", &self.pin_dm) | ||
.field("pin_dp", &self.pin_dp) | ||
.finish() | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is out of date. how about just using the Debug
derive for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We actually don not use the pins for anything once they are put into the struct. As in we have no release
method. Should I remove them from the struct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fields `pin_dm` and `pin_dp` are never read
`UsbDevice` has a derived impl for the trait `Debug`, but this is intentionally ignored during dead code analysis
`#[warn(dead_code)]` on by default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. Latest changes look good.
src/usb.rs
Outdated
fn format(&self, f: defmt::Formatter) { | ||
defmt::write!( | ||
f, | ||
"Peripheral {{ usb: USB, pin_dm: {}, pin_dp: {}}}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Name is out of date
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome. Thanks for doing this! Looks good to me.
No description provided.